Skip to content

Fix iteration of OpenAL src.bufQueue #23916

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 14, 2025

Conversation

JoeOsborn
Copy link
Contributor

This failing test shows up under the interactive tests only, so I guess CI didn't catch it. Caused by #23867 .

@JoeOsborn
Copy link
Contributor Author

I skimmed that PR and it looks like this is the only case where .length wasn't deleted from the iterated sequence during the conversion to for..of.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I wonder if we could make one of the non-interactive tests cover this?

@JoeOsborn
Copy link
Contributor Author

I wonder---doesn't AudioContext stuff require a click? I don't know how it acts or what it does in the absence of interaction.

@sbc100
Copy link
Collaborator

sbc100 commented Mar 13, 2025

I wonder---doesn't AudioContext stuff require a click? I don't know how it acts or what it does in the absence of interaction.

The CI tests we recently updated to avoid that requirement. See #23665 and #23701

@JoeOsborn
Copy link
Contributor Author

I've copied one or two tests and given a "fast" version of the test_openal_buffers to avoid having to cycle through the whole entertainer.wav in automated tests.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

})
def test_openal_playback(self, args):
shutil.copy(test_file('sounds/audio.wav'), '.')
self.btest('openal/test_openal_playback.c', '1', emcc_args=['-O2', '--preload-file', 'audio.wav'] + args)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't use use best_exit here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure we could! I just copied this from test_other.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this test doesn't seem to work in browser under pthreads (the sound clip finishes but the test never terminates), but that's not really related to this PR. Any ideas on why that might be happening? Does the thread need to do some sleeps or something?

In the meantime I have removed the pthread parameterization of this test on test_browser.py.

@JoeOsborn
Copy link
Contributor Author

The rebaseline on this branch was a mistake, do you want me to undo it?

@@ -1 +1 @@
232600
232604
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rebase just landed separately: #23918

@JoeOsborn JoeOsborn force-pushed the fix-openal-iteration branch from 669dfaf to 2fb2d90 Compare March 13, 2025 22:13
@sbc100 sbc100 enabled auto-merge (squash) March 13, 2025 22:30
@JoeOsborn
Copy link
Contributor Author

The test failure is that emscripten_async_call needs to cast an ALuint to a void *, but void * is bigger than ALuint on 64-bit. Should I copy it into a intptr_t first and then cast to void *?

auto-merge was automatically disabled March 14, 2025 18:17

Head branch was pushed to by a user without write access

@JoeOsborn JoeOsborn force-pushed the fix-openal-iteration branch 2 times, most recently from 975ae94 to 8c23ffc Compare March 14, 2025 18:20
@JoeOsborn JoeOsborn force-pushed the fix-openal-iteration branch from 8c23ffc to 7de129f Compare March 14, 2025 19:26
@sbc100 sbc100 merged commit da0ae80 into emscripten-core:main Mar 14, 2025
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants